🧹 Decouple HTTP exceptions from VAD utility - #11338
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e3840d6f-32c9-432e-b77a-80cf8f24a3c8) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 921c6fcf4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except VADEmptyError: | ||
| raise HTTPException(status_code=400, detail="Audio is empty") |
There was a problem hiding this comment.
When VAD finds no speech, this commit changes the utility's exception contract and adds translations at the HTTP and batch-script boundaries, but it modifies no tests; an incorrect exception type or catch would therefore turn the upload's intended 400 into a 500 or let the maintenance thread fail unnoticed. Add a behavioral test that drives the zero-segment path and verifies VADEmptyError is translated correctly, as required for behavior-changing bug fixes.
AGENTS.md reference: AGENTS.md:L26-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the zero-segment path is now covered end-to-end — test_empty_vad_returns_400_not_500 drives the real apply_vad_for_speech_profile through the upload route and asserts the 400 (no upload); test_apply_vad_for_speech_profile_raises_for_zero_segments asserts the real utility raises VADEmptyError; test_batch_skips_empty_vad_without_uploading covers the script boundary. All pass (34/34).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- In
backend/routers/speech_profile.py, theVADEmptyError→400 "Audio is empty"branch is currently untested, so a future change could silently alter this user-facing contract (for example returning a generic error instead of the expected 400). Add a focused router test that triggersVADEmptyErrorand asserts the exact status code/message to de-risk regressions.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/routers/speech_profile.py">
<violation number="1" location="backend/routers/speech_profile.py:86">
P3: The new `VADEmptyError` handling path is untested. The router branch that converts `VADEmptyError` into the `400 "Audio is empty"` response is the exact user-facing behavior this PR preserves, and the script's `except VADEmptyError: return` is the new robustness behavior that motivated the change — but neither has a regression test (existing tests only cover the WAV decode/duration failure branches, and nothing asserts the empty-audio path). Consider adding a small unit test that patches `apply_vad_for_speech_profile` to raise `VADEmptyError` and asserts the router returns 400 without uploading, and one for the script's early-return-on-empty branch.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| apply_vad_for_speech_profile(file_path) | ||
| try: | ||
| apply_vad_for_speech_profile(file_path) | ||
| except VADEmptyError: |
There was a problem hiding this comment.
P3: The new VADEmptyError handling path is untested. The router branch that converts VADEmptyError into the 400 "Audio is empty" response is the exact user-facing behavior this PR preserves, and the script's except VADEmptyError: return is the new robustness behavior that motivated the change — but neither has a regression test (existing tests only cover the WAV decode/duration failure branches, and nothing asserts the empty-audio path). Consider adding a small unit test that patches apply_vad_for_speech_profile to raise VADEmptyError and asserts the router returns 400 without uploading, and one for the script's early-return-on-empty branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/speech_profile.py, line 86:
<comment>The new `VADEmptyError` handling path is untested. The router branch that converts `VADEmptyError` into the `400 "Audio is empty"` response is the exact user-facing behavior this PR preserves, and the script's `except VADEmptyError: return` is the new robustness behavior that motivated the change — but neither has a regression test (existing tests only cover the WAV decode/duration failure branches, and nothing asserts the empty-audio path). Consider adding a small unit test that patches `apply_vad_for_speech_profile` to raise `VADEmptyError` and asserts the router returns 400 without uploading, and one for the script's early-return-on-empty branch.</comment>
<file context>
@@ -81,7 +81,10 @@ def upload_profile(file: UploadFile, uid: str = Depends(auth.get_current_user_ui
- apply_vad_for_speech_profile(file_path)
+ try:
+ apply_vad_for_speech_profile(file_path)
+ except VADEmptyError:
+ raise HTTPException(status_code=400, detail="Audio is empty")
</file context>
There was a problem hiding this comment.
Addressed: test_empty_vad_returns_400_not_500 asserts status 400 + detail "Audio is empty" and no upload; test_batch_skips_empty_vad_without_uploading covers the script early-return branch. Both pass.
undivisible
added
human
Git-on-my-level
removed
the
workflow-review
|
Reviewed the full three-file diff; this looks like a good separation-of-concerns cleanup.
Backend CI is green. I also removed Automated maintainer review; final merge decision remains with the maintainers. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
the
positive-signal
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_644ec186-dc8a-499c-849d-3e7693c026c9) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1665c14c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) as mock_aseg, patch.object(mod, "VADEmptyError", _FakeVADEmptyError), patch.object( | ||
| mod, "apply_vad_for_speech_profile", side_effect=_FakeVADEmptyError("Audio is empty") |
There was a problem hiding this comment.
Fresh evidence since the earlier missing-coverage comment is that this new test replaces both VADEmptyError and apply_vad_for_speech_profile with matching fakes, so it never executes the changed zero-segment branch in utils/stt/vad.py. If the real utility continued raising HTTPException or later raised another type, this test would still pass while silent uploads return 500 and the batch script fails to handle them; patch vad_is_empty to return [] and invoke the real utility through the route (or separately assert the real utility's exception) so the regression test covers the production contract.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the test now patches vad_is_empty to return [] and runs the real apply_vad_for_speech_profile through the route (test_empty_vad_returns_400_not_500), plus a direct utility-level assertion in test_vad_onnx.py. No fakes of the exception or the utility; a wrong exception type in vad.py would now fail the suite.
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Introduces VADEmptyError so empty speech-profile audio returns 400 instead of 500, with a unit test. CI green. Approve-only (refactor, no linked bug issue).
|
Thanks for the update — I reviewed the current four-file diff on this head, including the newly added regression test.
Validation: backend CI is green. I also ran This still looks like a clean backend separation-of-concerns cleanup, and the existing by AI on behalf of David — automated maintainer review; final merge decisions remain with the maintainers. |
4e1665c to
5466314
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_892655a0-1a99-445d-ad8c-1c9f5ad4013d) |
5466314 to
84043ba
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_20ce6641-379a-4e88-b712-b418987d762b) |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks — this looks like a good narrow backend cleanup. I reviewed the current head (84043baef77fa1e23442ad297539e9fc2ec8f9fa):
backend/utils/stt/vad.py: replacing the FastAPIHTTPExceptionwithVADEmptyErrorkeeps the VAD utility framework-independent while preserving the empty-audio signal before any trimming/export work.backend/routers/speech_profile.py: catchingVADEmptyErrorat the upload route preserves the user-facing400 "Audio is empty"response and still bails out before duration caching, upload, or embedding extraction.backend/scripts/stt/j_apply_vad_to_speech_profiles.py: handlingVADEmptyErrorlets the maintenance script skip empty profiles instead of crashing on a web-layer exception, which matches the decoupling goal.backend/tests/unit/test_speech_profile_wav_decode.py: the added empty-VAD test asserts the 400 response and verifies upload is not called, covering the earlier automated concern around this route behavior.
CI is green, and I do not see a blocking issue in this diff. Keeping this as a positive signal; final merge judgment can stay with the human maintainer path already in progress.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ceb5afbc-f18d-4c7f-bd6a-208ae658e701) |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3e4715d46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(value, str) and value: | ||
| try: | ||
| action_item_data[date_field] = datetime.fromisoformat(value.replace('Z', '+00:00')) |
There was a problem hiding this comment.
When a tool or LLM supplies a date-only ISO string such as 2024-01-01, datetime.fromisoformat produces a timezone-naive value; an already-parsed naive datetime bypasses this branch entirely. Those values are then handed to Firestore, which rejects them, causing extracted action items—potentially an entire background batch—to be lost. Convert both string and datetime inputs to timezone-aware UTC before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| # Manual only: automatic push/schedule tagging was cutting a new macOS candidate | ||
| # tag on nearly every desktop-affecting main merge and every 15 minutes. Plan and | ||
| # publish a candidate deliberately via workflow_dispatch; qualification/promotion |
There was a problem hiding this comment.
After this edit, desktop_auto_release.yml has only workflow_dispatch, so ordinary merges never invoke the planner and no daily beta candidate is cut unless an operator remembers to run the workflow manually. Restore the scheduled automatic trigger required by the repository's desktop release pipeline.
AGENTS.md reference: AGENTS.md:L126-L128
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| if url == '' or url == ',': | ||
| disable_user_webhook_db(uid, wtype) | ||
| else: | ||
| enable_user_webhook_db(uid, wtype) |
There was a problem hiding this comment.
When a mobile user clears the audio-bytes webhook and saves, developer_mode_provider.dart posts ,<delay> (normally ,5), which does not match either literal here. The endpoint therefore re-enables a webhook with no URL, so the toggle comes back on and audio processing remains enabled despite the user's attempt to disable it; determine emptiness from the URL portion before the comma.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| String? selectedLanguageName = selectedLanguage != null | ||
| ? homeProvider.availableLanguages.entries.firstWhere((element) => element.value == selectedLanguage).key |
There was a problem hiding this comment.
If the stored primary-language code is not in this bundled map, opening the forced language dialog throws StateError at firstWhere before it can render. This is reachable for valid server-supported values such as sw, cy, or af, which PATCH /v1/users/language accepts but the bundled map omits; use the existing safe lookup/fallback instead of assuming every stored code is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| setSegments((prev) => { | ||
| // Update existing segment or add new one | ||
| const existingIndex = prev.findIndex((s) => s.id === segment.id); | ||
| if (existingIndex >= 0) { | ||
| const updated = [...prev]; | ||
| updated[existingIndex] = segment; | ||
| return updated; | ||
| } | ||
| return [...prev, segment]; |
There was a problem hiding this comment.
During long browser recordings, every finalized segment takes this append path, so the React state and rendered transcript grow without limit while each update copies the entire array. Sessions reaching hundreds of segments progressively stall Chrome and can make the recording UI unusable; retain only a bounded recent window while leaving the server-side session intact for finalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| conversation_ids = vector_db.query_vectors(query, uid, starts_at=starts_at, ends_at=ends_at, k=limit) | ||
| if not conversation_ids: | ||
| return [] |
There was a problem hiding this comment.
This route now queries only conversation summary vectors, which do not contain phrases spoken solely in transcript segments. Consequently, MCP clients receive no result for exact names, decisions, or quotes that were omitted from the generated summary even though matching transcript chunks are indexed; merge search_transcript_chunks hits with the summary results and return the corresponding snippets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| // Content-derived hit region: the fixed window is larger than the | ||
| // visible chrome/menu, and its transparent margins must keep passing | ||
| // clicks through to windows below (hitTest returns nil outside this). |
There was a problem hiding this comment.
In notch mode the panel is intentionally fixed at its maximum hover size, leaving a large transparent area around the visible chrome. Returning nil from the content view's hitTest does not make the NSWindow click-through—the frame view still owns the event—and this change removes the window-level ignoresMouseEvents synchronization, so the invisible panel intercepts clicks on the main window's top navigation and other apps beneath it. Restore window-level mouse interception control or stop reserving the oversized transparent frame.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| ] | ||
| for f in futures: | ||
| f.result() | ||
| ordered_chunks = [context_data[m.id] for m in memories if m.id in context_data] | ||
| context_str = '\n'.join(ordered_chunks).strip() | ||
| context_str = '\n'.join(context_data.values()).strip() |
There was a problem hiding this comment.
Each worker inserts its chunk into context_data when it finishes, so joining dict.values() orders context by nondeterministic thread completion rather than the similarity-ranked memories list established above. With multiple retrieved conversations this can present the LLM with arbitrarily reordered evidence and produce unstable or less relevant answers; rebuild the output by iterating memories and selecting matching IDs instead of deferring the known defect in an untracked TODO.
AGENTS.md reference: AGENTS.md:L93-L93
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| @override | ||
| @EnviedField(varName: 'OPENAI_API_KEY', obfuscate: true) | ||
| final String? openAIAPIKey = _ProdEnv.openAIAPIKey; |
There was a problem hiding this comment.
The production Codemagic app workflows write the real OPENAI_API_KEY into .env, and this EnviedField causes build_runner to compile that value into every released Flutter binary. obfuscate: true is reversible obfuscation rather than secret storage, so an app recipient can recover the provider credential and use it outside Omi; this also directly contradicts app/config/client_env_policy.yaml, which classifies OPENAI_API_KEY as server-only. Route OpenAI calls through an authenticated backend and remove the field from the public client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
| async def initialize_stt(self) -> bool: | ||
| request = self.host.request | ||
| provider = getattr(self.host.stt_service, 'value', self.host.stt_service) | ||
| if self.host.use_custom_stt: |
There was a problem hiding this comment.
When a session starts with Parakeet but _create_stt_socket falls back to Modulate, that method updates self.host.stt_service after this local value has already been captured. The death monitor and initialization failure paths therefore report parakeet in the client failure event and provider metrics even though Modulate was serving and failed, obscuring the actual incident and misleading provider-specific diagnostics; resolve the provider after socket creation or at each failure boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Out of scope: this file is not part of this PR's diff. The branch was reset to the VAD-only exception-decoupling change (per the maintainer review), and the tree-wide diff that surfaced this file was removed.
Git-on-my-level
added
needs-scope-reduction
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the original VAD cleanup — the small VADEmptyError part still looks directionally right (backend/utils/stt/vad.py, backend/routers/speech_profile.py, and backend/scripts/stt/j_apply_vad_to_speech_profiles.py keep the framework exception at the route/script boundary).
I need to request changes on the current head, though, because it has grown far beyond that refactor and now carries several unrelated, high-risk behavior changes:
app/lib/backend/http/openai.dartadds direct client calls tohttps://api.openai.com/v1/...withAuthorization: Bearer ${Env.openAIAPIKey}, andapp/.env.template/app/lib/env/*.dartadd an app-side OpenAI key. That moves LLM calls and user content from the backend-managed/gateway path into the shipped app surface, which needs an explicit security/privacy/product decision rather than being bundled into a VAD utility cleanup.backend/routers/desktop_chat.pyremoves the structured managed lane (CHAT_STRUCTURED_AUTO_LANE_ID,_MANAGED_STRUCTURED_ALIASES, lane-specific accounting) and routes managed gateway requests through the chat-agent lane. That can change model routing/personality/accounting for non-conversational extraction/planner calls.backend/route_policy_manifest.yamlplus routers such asbackend/routers/conversations.py,backend/routers/knowledge_graph.py,backend/routers/integrations.py,backend/routers/memories.py, andbackend/routers/users.pyremove multiple first-party extraction/synthesis/language endpoints and their policy entries, while many corresponding tests are deleted. That is a broad API/product contract change, not a decoupling refactor..github/workflows/desktop_auto_release.yml,.github/scripts/check-desktop-changelog.py,.github/scripts/plan-desktop-release.py, and their tests remove scheduled release-train/changelog/fallback behavior. This is release infrastructure and needs focused workflow review on its own.desktop/macos/AGENTS.mdchanges AI/coding-agent release-pipeline guidance, but the new text says candidates are cut on every macOS-affecting merge plus a 15-minute schedule, while the workflow diff removes the schedule and leavesworkflow_dispatchonly. That instruction file can directly mislead future coding/review agents about how desktop releases work, so it should not land in this state.backend/database/vector_db.pyremoves the injectablequery_vectorpath and weakens transcript date filtering to only apply when both bounds are present. That changes retrieval behavior and testability independently of the VAD work.web/app/package.json/web/app/package-lock.jsonremove Vitest and test scripts while web transcript tests are deleted, which further broadens the risk surface.
Please reduce this PR back to the VAD exception cleanup, or split/rework these unrelated backend API, desktop release, client-LLM, retrieval, web dependency, and agent-instruction changes into separately reviewed PRs with their own validation. The current head needs security-sensitive client/API-key review and release-workflow maintainer sign-off before it can be considered.
by AI on behalf of David — automated maintainer review; blocking here because the current head bundles unrelated security-sensitive client LLM/API-key changes, release workflow changes, and inaccurate agent-facing release guidance.
Refactored `apply_vad_for_speech_profile` in `backend/utils/stt/vad.py` to raise a new `VADEmptyError` instead of FastAPI's `HTTPException(400)`. This logic is now caught in the `backend/routers/speech_profile.py` router and `backend/scripts/stt/j_apply_vad_to_speech_profiles.py` script, decoupling web exceptions from core domain utilities. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Exercise the real VAD zero-segment exception at the utility boundary and cover the HTTP and batch translations at their callers. Failure-Class: none Tests: backend/.venv/bin/python -m pytest -q tests/unit/test_speech_profile_wav_decode.py tests/unit/test_vad_onnx.py
Exercise the production VAD zero-segment exception through the upload route and batch maintenance script, asserting both user-facing translations and their side-effect guards. Failure-Class: none
a3e4715 to
ba4e39e
Compare
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a49a248c-0bf3-4aa5-bf22-ba9a46d46cb5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba4e39e538
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) as mock_upload, patch.object( | ||
| vad_mod, "vad_is_empty", return_value=[] | ||
| ) as mock_vad: | ||
| batch_mod.execute() |
There was a problem hiding this comment.
Fresh evidence since the earlier coverage comments is that this added test invokes the failure path through threading.Thread: if the new except VADEmptyError block is removed or mistyped, the exception terminates only the worker, join() still returns, and both assertions still pass (pytest merely reports an unhandled-thread warning by default). Run the worker synchronously through a controllable seam or otherwise assert that no worker exception occurred so the regression test actually protects the batch exception boundary.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
Git-on-my-level
added
backend
Resolved on current head: prior unrelated client-LLM/API-key, release workflow, API-removal, retrieval, web-test, and agent-instruction changes are no longer present.
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for narrowing this back down. I reviewed the current head (ba4e39e53854fc573e142dd21af031d6a3f87f03), and the earlier blocking concerns about unrelated client-LLM/API-key, release workflow, API-removal, retrieval, web-test, and agent-instruction changes appear resolved on this head.
File-by-file notes:
backend/utils/stt/vad.py:apply_vad_for_speech_profile()now raises a domain-specificVADEmptyErrorinstead of importing/raising FastAPI'sHTTPException, so the VAD utility is no longer coupled to the web layer while preserving the empty-audio signal before trimming/export.backend/routers/speech_profile.py: the upload route catchesVADEmptyErrorand translates it back to the existing400 "Audio is empty"API response, so client-facing behavior remains appropriate at the FastAPI boundary.backend/scripts/stt/j_apply_vad_to_speech_profiles.py: the batch speech-profile script now treats empty VAD results as a per-user skip instead of letting a web-framework exception crash the worker path, which is the right separation for this script context.backend/tests/unit/test_speech_profile_wav_decode.py: the new coverage exercises both the route translation path and the batch-script skip path, and keeps the upload side-effect guarded when VAD returns no segments.backend/tests/unit/test_vad_onnx.py: the new unit test verifies the utility-level contract directly: zero VAD segments raiseVADEmptyError.
Validation I ran locally: focused backend unit suite for test_speech_profile_wav_decode.py and test_vad_onnx.py — 34 passed.
I’m dismissing my stale blocking review from the broader previous head and leaving this as a positive signal rather than formal approval because the cached approval guard for this run disallows auto-approval.
by AI on behalf of David — automated maintainer review; leaving a positive signal after verifying the previous broad-scope/security/workflow concerns are no longer present on this narrowed backend cleanup.
…n-coupling-5064561204901573843 # Conflicts: # backend/utils/stt/vad.py
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_231e5518-4166-48ee-9bfe-3a3811397e4f) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_db2f1de8-786b-465e-83e6-99632c77241b) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aeb9888a78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from scripts.stt import j_apply_vad_to_speech_profiles as batch_mod | ||
| from utils.stt import vad as vad_mod |
There was a problem hiding this comment.
When backend tests run in the supported shared-process mode (BACKEND_PYTEST_FILE_ISOLATION=0), these imports cache batch_mod and the real vad_mod while database and storage dependencies resolve to _AutoMock; restoring sys.modules afterward does not repair those modules' captured references. Later tests therefore receive production modules permanently bound to fake Redis/storage functions, making results collection-order-dependent. Import these modules outside this module-scope stub window or replace the stubs through an isolated fixture.
AGENTS.md reference: backend/AGENTS.md:L216-L216
Useful? React with 👍 / 👎.
3 participants
🎯 What: The
apply_vad_for_speech_profileutility function inbackend/utils/stt/vad.pywas directly raising a FastAPIHTTPException(400)when a processed audio file was empty. This has been replaced with a domain-specificVADEmptyError. TheHTTPExceptionlogic has been moved up to the routing layer inbackend/routers/speech_profile.py. The background scriptbackend/scripts/stt/j_apply_vad_to_speech_profiles.pywas also updated to catchVADEmptyError.💡 Why: This change separates concerns and decouples domain/utility logic from web framework-specific exceptions.
HTTPExceptionshould only be thrown from routing layers. This makes the utility function safer to reuse across different contexts, such as thej_apply_vad_to_speech_profiles.pybackground script which previously would have crashed upon encountering an empty audio file.✅ Verification: Ran pytest tests against the updated files (
backend/tests/unit/test_speech_profile_wav_decode.py,backend/tests/unit/test_user_speaker_embedding.py,backend/tests/unit/test_vad_onnx.py) to confirm no regressions and tested API compatibility. Code Review was completed.✨ Result: A cleaner architectural separation of concerns between core utility code and web layers, and a more robust background script that won't crash when encountering empty voice segments.
PR created automatically by Jules for task 5064561204901573843 started by @undivisible
Note
Low Risk
Layering change with preserved upload 400 semantics; limited to speech-profile VAD paths and covered by new unit tests.
Overview
Speech-profile VAD no longer raises FastAPI errors from the STT utility layer. When VAD finds no speech segments,
apply_vad_for_speech_profilenow raises a domainVADEmptyErrorinstead ofHTTPException(400), and the FastAPI dependency onvad.pyis removed.The
POST /v3/upload-audiohandler catchesVADEmptyErrorand still returns 400 with detail "Audio is empty", so client behavior for empty/silent uploads stays the same while upload does not proceed to storage.The batch script
j_apply_vad_to_speech_profilescatchesVADEmptyError, logs, and skips that user instead of failing the job thread. Unit tests cover the router mapping, batch skip behavior, and the utility raisingVADEmptyError.Reviewed by Cursor Bugbot for commit aeb9888. Configure here.